[WIP] Conditions Hierarchy - #444
Conversation
… SigningCoordinator contract.
…or cohort participant.
…note this is specific to the same chain as the signing coordinator.
…s are not allowed.
…being invalid - need to improve upgrade process through reinitializers next time.
…e on Ethereum with SigningCoordinatorChild contracts on L2s.
…olia <-> Base Sepolia.
…dispatcher and registration of bridges / SigningCoordinatorChild contract specific to chain.
…ning coordinator dispatcher, and the creation of the multisig on the same chain. (next will be for cross-chain).
…on either side of the bridge.
…cohort on other chains via bridges. Bridge logic is abstracted away in the dispatcher contract which was updated for L1->L2 sender/receiver interfaces. Update signing coordinator tests accordingly.
Add test for cross-chain deployment. (Need to figure out why multisig addresses end up being different).
…address when registering and the chain is the current chain. Add tests for SigningCoordinatorDispatcher.
Revamped `lynx` contract deployments for signing infrastructure
`ThresholdSigningMultisig` tests
More testing of Signing Contracts
There was a problem hiding this comment.
Pull request overview
This is a Work-In-Progress (WIP) PR that introduces a comprehensive signing infrastructure called "Conditions Hierarchy" for the TACo network. The PR adds support for threshold signing multisig wallets that can be deployed across multiple chains, enabling cohort-based signature collection with cross-chain capabilities.
Key Changes
- New Signing Coordinator Infrastructure: Introduces
SigningCoordinator,SigningCoordinatorChild, andSigningCoordinatorDispatchercontracts to manage signing cohorts with cross-chain deployment support - Threshold Signing Multisig: Implements ERC-1271 compliant threshold multisig wallet with clone factory pattern for efficient deployment
- Cross-Chain Bridge Support: Adds Optimism-based L1/L2 bridge contracts (
OpL1Sender,OpL2Receiver) for cross-chain communication - Testing & Scripts: Comprehensive test coverage for new contracts and deployment/management scripts for lynx testnet
Reviewed changes
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| contracts/contracts/coordination/SigningCoordinator.sol | Main coordinator contract for managing signing cohorts with multi-chain support |
| contracts/contracts/coordination/ThresholdSigningMultisig.sol | ERC-1271 threshold multisig implementation with signer management |
| contracts/contracts/coordination/ThresholdSigningMultisigCloneFactory.sol | Factory for deterministic multisig clone deployment |
| contracts/contracts/coordination/SigningCoordinatorChild.sol | Child contract for deploying multisigs on target chains |
| contracts/contracts/coordination/SigningCoordinatorDispatcher.sol | Dispatcher for routing calls to appropriate chains |
| contracts/contracts/coordination/OpL1Sender.sol | L1 sender for Optimism bridge communication |
| contracts/contracts/coordination/OpL2Receiver.sol | L2 receiver for processing bridge messages |
| contracts/xchain/PolygonChild.sol | Added payable modifier to fallback function |
| tests/test_threshold_signing_multisig.py | Comprehensive tests for multisig functionality |
| tests/test_signing_coordinator.py | Tests for signing coordinator and cross-chain operations |
| scripts/form_signing_cohort.py | Script to initiate new signing cohorts |
| scripts/set_signing_cohort_conditions.py | Script to set conditions for cohorts |
| scripts/signing_cohort_additional_chain.py | Script to deploy cohort to additional chains |
| scripts/verify.py | Enhanced to support multiple contract verification |
| scripts/lynx/*.py | Deployment scripts for lynx testnet infrastructure |
| src/index.ts | Added SigningCoordinator to contract registry |
| test/registry.spec.ts | Updated tests for SigningCoordinator registry |
| requirements.txt | Updated Python dependencies |
| ape-config.yaml | Increased test account count to 70 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| require( | ||
| signingCohort.authority == msg.sender || serverAdmin.active, | ||
| signingCohort.authority == msg.sender || server.activeServerAdmins[msg.sender], |
There was a problem hiding this comment.
Does this mean that the cohort authority can override a server admin for setting the serverId-specific condition? If so, I don't think we want to give the cohort authority that power.
Only the server admin can set the serverId-specific condition. (cc @arjunhassard )
There was a problem hiding this comment.
But cohort admin can set itself to server admin anyway, right?
There was a problem hiding this comment.
You're right that technically they can take that route to set a serverid-specific condition.
Typically the cohort admin and server admins should be separate. They may be the same in Collab.Land's Discord server, but shouldn't be for other Discord servers.
There was a problem hiding this comment.
Removed privilege for cohort admin
| emit SigningCohortConditionsSet(cohortId, msg.sender, chainId, conditions); | ||
| } | ||
|
|
||
| function getUnsignedTransactionHash( |
There was a problem hiding this comment.
"If" we go this route (of getting conditions signed by the cohort), would a similar function to getSigningCohortDataHash(...) (which we use for the ritual) be simpler? I'm particularly worried about the use of nonce and that multisig function. wdyt?
There was a problem hiding this comment.
yeah, it can be without nonce similar to getSigningCohortDataHash, but it will open ability to execute same tx twice which in that case should not be big deal
There was a problem hiding this comment.
Removed nonce and simplified in general
| uint256[20] gap; | ||
| } | ||
|
|
||
| struct Server { |
There was a problem hiding this comment.
I think we should find a more generic term for Server, which is too specific to a particular use case. What about something like Domain?
|
|
||
| require( | ||
| signingCohort.authority == msg.sender || serverAdmin.active, | ||
| signingCohort.authority == msg.sender || server.activeServerAdmins[msg.sender], |
There was a problem hiding this comment.
But cohort admin can set itself to server admin anyway, right?
| ) external { | ||
| SigningCohort storage signingCohort = signingCohorts[cohortId]; | ||
| conditionsCheck(signingCohort, chainId); | ||
| require(serverId > 0, "Server id can't be zero"); |
There was a problem hiding this comment.
some optimization for getSigningCohortConditions
Type of PR:
Required reviews:
What this does:
Issues fixed/closed:
Why it's needed:
Notes for reviewers: